Allow Closures to reuse their run_time_cache when opcache is enabled #18556
Conversation
There was a problem hiding this comment.
Not only is it a CPU time regression, but it also leaks memory on the arena (it's just not reported as a leak, because arena).
Every time the scope changes (e.g. an inherited function called on different extending objects), a new run time cache is allocated.
If you want to meaningfully do this, you'll have to store the closures run_time_cache in e.g. class_mutable_data of the scope they belong to.
Otherwise, I think Dmitrys suggestion was to do this just once. I.e. store the scope for the first time the runtime cache is allocated (i.e. when ZEND_MAP_PTR_GET() is NULL) and otherwise heap allocate.
This would still cause repeated observer initializations, but make it consistent between opcache and non-opcache.
|
Yeah, I planned to analyze the regression and check for mistakes, because I didn't expected that. My goal was only to have a similar reuse rate with opcache than without, but indeed there may be ways to improve that even more.
I'm not sure what you mean here. Do you have an example where the scope of a closure changes? (without binding it explicitly). All I can think of is traits, but even in that case I allocate on the arena only once. With the current changes, I never allocate on the arena more than once per Closure.
That's what I was targeting, but it's possible I've made mistakes that prevent this. |
I see what I missed - the !ptr and the closure check are on two different lines and missed that they belong to the same condition. |
Possible alternative fix for GH-10782
This implements the approach described in #10782 (comment):
Unfortunately this results in a small regression on the Symfony benchmark (0.20% wall time, 0.03% icount).